Enable check for using GC on Image in unsupported use cases #1979

ShahzaibIbrahim · 2025-04-02T10:40:04Z

Logging a warning for GC initialized with unsupported use cases.

all dynamic images (retrieved via any of the existing providers).
all images for which handles in other zoom values have already been created.

Examples

Run this examples with following VM arguments

-Dswt.autoScale=quarter
-Dswt.autoScale.updateOnRuntime=true

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class SWTGCImageExample {
	static final ImageFileNameProvider filenameProvider = zoom -> {
		String path = null;
		switch (zoom) {
		case 150:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
			break;
		case 200:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
			break;
		default:
			path = "bin/org/eclipse/swt/snippets/eclipse.png";
		}
		return path;
	};

    public static void main(String[] args) {
    	 Display display = new Display();
         Shell shell = new Shell(display);

         Image image = new Image(display, filenameProvider);
         GC gc = new GC(image);

         // Draw something on the image
         gc.setForeground(display.getSystemColor(SWT.COLOR_RED));
         gc.drawRectangle(50, 50, 100, 100);
         gc.drawText("Hello SWT!", 60, 90);

         // Dispose GC after drawing
         gc.dispose();

         // Dispose image
         image.dispose();

         shell.dispose();
         display.dispose();
    }
}

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.widgets.*;

public class SWTImageHandleExample {
    public static void main(String[] args) {
        Display display = new Display();

        // Create an image using the display
        Image image = new Image(display, 200, 200);

        // Obtain the handle from the Image using win32_getHandle
        long handle = image.win32_getHandle(image, 100);
        System.out.println("Created image handle: " + handle);

        // Initialize GC with the image
        GC gc = new GC(image);

        // Modify the image using GC
        gc.setBackground(display.getSystemColor(SWT.COLOR_BLUE));
        gc.fillRectangle(50, 50, 100, 100);

        // Dispose GC and Image
        gc.dispose();
        image.dispose();

        display.dispose();
    }
}

github-actions · 2025-04-02T10:51:44Z

Test Results

545 files ±0 545 suites ±0 31m 50s ⏱️ - 8m 3s
4 377 tests ±0 4 359 ✅ ±0 18 💤 ±0 0 ❌ ±0
16 647 runs ±0 16 506 ✅ ±0 141 💤 ±0 0 ❌ ±0

Results for commit 3f2b53c. ± Comparison against base commit f498ca3.

♻️ This comment has been updated with latest results.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

laeubi

I don't think that printing to system out is appropriate here.

Either it is an error, then we should throw an exception or it should simply be ignored.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

laeubi

It looks now better using the error handler, but I think the message must be enhanced, e.g. it should explain what is the alternative to use instead.

Just a assume a common use-case, where I load an Image from a file, not I want to draw some test/watermark on it and save it again. How is this supposed to work? At best we add a new snippet that shows best practice on how to perform image modifications "the supported way" and then reference it in the message.

ShahzaibIbrahim · 2025-05-06T13:07:37Z

@laeubi I have added a snippet to demonstrate on how to use Image(Display, ImageGcDrawer, int, int) to load an image to the system and draw watermark over it (supported for all zoom level images.

Also I have again changed the condition to only log an error when using the strict mode. This was done as per @HeikoKlare suggestion since Display#setRuntimeExceptionHandler handler contract states to only use it whenever an exception is thrown by a listener or external callback function and this is not the case here.

HeikoKlare

I have some comments but in general I am fine with the proposed change.

Also I have again changed the condition to only log an error when using the strict mode. This was done as per @HeikoKlare suggestion since Display#setRuntimeExceptionHandler handler contract states to only use it whenever an exception is thrown by a listener or external callback function and this is not the case here.

To clarify this: it's not (only) about the specified usage scenario of the runtime exception handler but also about how this debug information may be used. I don't see that anyone will inspect the error log view of tested products, configure a different handler for runtime exceptions at the display to find such errors or the like, so the logging capability will likely remain unused. There is already some other logging guarded behind the strictChecks property, which one can just activate for a product under test to then inspect the logs of, e.g., Jenkins builds, test runs etc.
We may think of improving the way in which the collection of debug information for strictChecks is done in general, but since all other places currently use standard output, we should stick with it here to have a uniform way of logging that information and may do that as a separate step for all affected places.

examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet384.java

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

laeubi · 2025-05-07T04:50:32Z

I don't see that anyone will inspect the error log view of tested products

Whenever something goes wrong I go to the error log and I also report incidents from there, others do as well, so I don't see this a general problem.

configure a different handler for runtime exceptions at the display to find such errors or the like

Also SWT can be used standalone so its hard to tell how people actually use it. In any case if no one is looking at the errors, then my initial recommendation stays: simply don't log this. If it is important enough for people, they will do.

HeikoKlare · 2025-05-07T08:25:56Z

Whenever something goes wrong I go to the error log and I also report incidents from there, others do as well, so I don't see this a general problem.

If we guard this behind strict checks (like we do for all comparable checks), it will only pop-up in the error log if you enable strict checks, and I guess that usually no one does not.

Also SWT can be used standalone so its hard to tell how people actually use it. In any case if no one is looking at the errors, then my initial recommendation stays: simply don't log this. If it is important enough for people, they will do.

The strict checks are an ability for developers to run an application with, as the name says, with more strict checks. You have to explicitly enable them to validate your application (manually or via automated tests) regarding those checks. The information is not that relevant for a consumer of an application (who sees the error log and may use the information to report a bug if it annoys them) but just for developers who can simply activate those strict checks to have further validation of an application. This is the same for the other existing strict checks. So why shouldn't we provide the ability to check an application regarding the risk of imperfect images or why should we introduce a different means of identifying them than the other strict checks we have?

With this change, warnings will be logged when strict checks are enabled and a GC is initialized for an image in unsupported cases: 1. images whose scaled variants are not derived via scaling from an original version but are retrieved from a central data source (like ImageDataProvider or ImageFileNameProvider) 2. images for which handles in other zoom values have already been created such that drawing on them will only be applied to one of those handles

HeikoKlare

I rebased on master resolving conflicts due to a snippet with the same name meanwhile being added. I also slightly adapted the implementation.

Change is now fine for me. @laeubi do you have any remaining concerns or can we merge this for M3?

laeubi

Looks good now. I assume there will be a N&N for SWT already to make users generally aware of ImageDrawer so this seems suitable here.

HeikoKlare · 2025-05-13T07:24:49Z

Thank you!
Yes, the N&N is from previous release: https://eclipse.dev/eclipse/news/news.html?file=4.35/platform_isv.html

ShahzaibIbrahim linked an issue Apr 2, 2025 that may be closed by this pull request

Log an error, when GC with Image in unsupported use cases vi-eclipse/Eclipse-Platform#204

Closed

ShahzaibIbrahim force-pushed the master-204 branch from a3d9ee5 to 84840bc Compare April 2, 2025 10:41

akoch-yatta reviewed Apr 2, 2025

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved

laeubi requested changes Apr 3, 2025

View reviewed changes

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java Outdated Show resolved Hide resolved

ShahzaibIbrahim marked this pull request as draft April 3, 2025 09:00

ShahzaibIbrahim mentioned this pull request Apr 23, 2025

Log an error, when GC with Image in unsupported use cases vi-eclipse/Eclipse-Platform#204

Closed

ShahzaibIbrahim force-pushed the master-204 branch 3 times, most recently from 6ae9927 to 0e1052e Compare May 2, 2025 14:01

ShahzaibIbrahim marked this pull request as ready for review May 2, 2025 14:03

ShahzaibIbrahim requested a review from laeubi May 2, 2025 14:14

laeubi requested changes May 2, 2025

View reviewed changes

ShahzaibIbrahim force-pushed the master-204 branch 2 times, most recently from 43c3c14 to 9b16a66 Compare May 6, 2025 13:03

HeikoKlare reviewed May 6, 2025

View reviewed changes

ShahzaibIbrahim force-pushed the master-204 branch 4 times, most recently from b045303 to b591dd8 Compare May 8, 2025 10:41

HeikoKlare force-pushed the master-204 branch 2 times, most recently from 6ed864d to 2c04d53 Compare May 12, 2025 15:17

HeikoKlare force-pushed the master-204 branch from 2c04d53 to 3f2b53c Compare May 12, 2025 15:18

HeikoKlare approved these changes May 12, 2025

View reviewed changes

HeikoKlare changed the title ~~GC with Image in unsupported use cases~~ Enable check for using GC on Image in unsupported use cases May 12, 2025

laeubi approved these changes May 13, 2025

View reviewed changes

HeikoKlare merged commit 0642bb4 into eclipse-platform:master May 13, 2025
17 checks passed

fedejeanne mentioned this pull request Jul 4, 2025

A strict check 200% image data is double the size of 100% image data #2249

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable check for using GC on Image in unsupported use cases #1979

Enable check for using GC on Image in unsupported use cases #1979

Uh oh!

ShahzaibIbrahim commented Apr 2, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 2, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

laeubi left a comment

Uh oh!

Uh oh!

laeubi left a comment

Uh oh!

ShahzaibIbrahim commented May 6, 2025

Uh oh!

HeikoKlare left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laeubi commented May 7, 2025

Uh oh!

HeikoKlare commented May 7, 2025

Uh oh!

HeikoKlare left a comment

Uh oh!

laeubi left a comment

Uh oh!

HeikoKlare commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

Enable check for using GC on Image in unsupported use cases #1979

Enable check for using GC on Image in unsupported use cases #1979

Uh oh!

Conversation

ShahzaibIbrahim commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Examples

Uh oh!

github-actions bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

ShahzaibIbrahim commented May 6, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laeubi commented May 7, 2025

Uh oh!

HeikoKlare commented May 7, 2025

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

HeikoKlare commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

ShahzaibIbrahim commented Apr 2, 2025 •

edited

Loading

github-actions bot commented Apr 2, 2025 •

edited

Loading